Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(protocol-designer): update heater shaker form fields #16580

Merged
merged 4 commits into from
Oct 23, 2024

Conversation

ncdiehl11
Copy link
Collaborator

@ncdiehl11 ncdiehl11 commented Oct 23, 2024

Overview

This PR overhauls the new PD heater shaker form. I consolidate shake duration form field (minutes and seconds) into a single time field. I also fix the function of the HS form toggle input fields and update errors, maskers, and default values for the form. I also add form-level HS form errors to catch required fields when they are toggled but no input is given.

Closes AUTH-864, Closes AUTH-966

Test Plan and Hands on Testing

  • create or import a PD protocol with heater shaker step
  • toggle each form field and test various inputs, checking that errors are properly caught

Changelog

  • implement consolidated heaterShakerTimer form field
  • fix behavior or heater shaker toggle input form fields
  • add errors for heater shaker form
  • abstract getTimeFromForm utility to work with heater shaker form in addition to pause form
  • update heater shaker form field copy

Review requests

  • see test plan

Risk assessment

medium. Modifies behavior of heater shaker form to args. I checked tests and exported protocols with the new consolidated timer field, and all looks correct.

This PR overhauls the new PD heater shaker form. I consolidate shake duration form field (minutes
and seconds) into a single time field. I also fix the function of the HS form toggle input fields
and update errors, maskers, and default values for the form.

Closes AUTH-864, Closes AUTH-966
@ncdiehl11 ncdiehl11 self-assigned this Oct 23, 2024
@ncdiehl11 ncdiehl11 marked this pull request as ready for review October 23, 2024 19:36
@ncdiehl11 ncdiehl11 requested a review from a team as a code owner October 23, 2024 19:36
@jerader
Copy link
Collaborator

jerader commented Oct 23, 2024

ran into a small bug

Screen.Recording.2024-10-23.at.15.52.58.mov

@ncdiehl11 ncdiehl11 requested a review from koji October 23, 2024 20:54
Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! nice work! i also tested and the timer command was generated as expected

Only thing is the toggles are smaller for some reason this time around. But the first time i looked at your branch, the toggles were the correct size, so idk what happened 🥲

Screenshot 2024-10-23 at 16 58 11

@koji
Copy link
Contributor

koji commented Oct 23, 2024

Screenshot 2024-10-23 at 4 58 37 PM

@ncdiehl11 ncdiehl11 merged commit 4887bd5 into edge Oct 23, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants